Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] State management updates #345

Closed
wants to merge 48 commits into from

Conversation

Parkreiner
Copy link
Contributor

@Parkreiner Parkreiner commented Sep 1, 2023

This update makes quite a few changes to developer ergonomics for the application. Special thanks to @tlovis for helping me out with this!

Major changes

Better type-safety for components consuming the Redux store

The Redux implementation has had a custom useAppSelector hook added to and exported from the store.ts file. It works exactly like the previous useSelector at runtime – there is actually zero difference in runtime functionality – but it gives you full type-safety and auto-complete support. No more risks of random any types or needing to guess about the structure of the store.

Every single file in the application has already been converted over, so future code updates should be quite a bit less painful.

There is no more need for this:

const isDark = useSelector((store: { ui: { isDark: boolean } }) => store.ui.isDark);

This is fully equivalent in functionality, and is even more type-safe:

// Correctly inferred as type boolean
const isDark = useAppSelector((store) => store.ui.isDark);

All top-level React Router components have been typed

This has certainly been one of the biggest pain points for the application. The current app architecture is still heavily based around container components and presentation components. That is, main.tsx, the main entry-point for the React Router setup, used Redux's connect API to get every single potentially-relevant state value and dispatch, and then directly passed them down to every single Router component.

While this does come with its own set of issues – the biggest one being that all state is bound to React's render cycles at all times, meaning that the app can over-render in surprising ways – it did mean that the props could be defined from one central place, and then exported for the other components to consume.

This process did highlight that not all components were consuming the values consistently, though. Because the other components went without type-checking for so long, there are some prop mismatches between components. However, there should be less of a need to blindly guess at what a given component has access to in the future. I fixed what props I could with the limited time I had for this cycle, but in the future, these prop mismatches will need to be corrected.

However, I do want to be clear: adding a centralized type is a band-aid solution. The ultimate goal should be to update each individual component to directly consume Redux via useAppSelector or useAppDispatch. These subscriptions should be "tamped down" to the lowest subtree component possible. Not only would this make it more clear where certain components are consuming values and clean up component API type, but it would also minimize which components need to re-render for any given state update.

One other note: because of how React's render cycles are structured, these re-render issues are all-or-nothing. As in, until every single Route component has been fully updated, an update to one will cause an update to the others, even if those others don't receive any props. If this becomes enough of an issue, those components that don't need to re-render can opt into React.memo.

mapStateToProps and mapDispatchToProps are almost completely removed

Aside from the main.tsx file mentioned earlier, every other component has been updated to use correctly-typed RTK hooks. main.tsx is the only file left, but realistically, it probably shouldn't have any Redux hooks imported once the app-wide restructuring has been finished.

Custom path aliases

Both the TSConfig and Webpack files have been updated to add support for custom import aliases, with full auto-complete support from the TS LSP.

Now instead of needing to do this:

// File 1
import { useAppDispatch } from "../toolkit-refactor/store";

// File 2
import { useAppDispatch } from "../../../.././toolkit-refactor/store";

You can import from the same absolute path for any component, no matter where you are:

// File 1 and file 2 can use this exact same line with no changes
import { useAppDispatch } from "~/toolkit/store";

All files that were consuming Redux have been updated to these imports. And because none of the files are directly importing from a specific file structure anymore, the toolkit-refactor folder has been renamed to just rtk.

I will note that any JS files still using require do not get the full benefits of this change. Using absolute paths should still work, because Webpack can still resolve the paths during the compilation step. But because these imports exist outside of TS, it can't give you auto-complete at author time.

Minor changes

Webpack files have improved type-safety

Even though the Webpack files have not been converted to TypeScript just yet, the existing files have been augmented with JSDoc comments, including the @ts-check directive. This should give better auto-complete support when adding features to the config files, and should notify you of (some) accidental typos.

Several files have been formatted

I'm hoping to make one other PR in the next few days about requiring a PR to be formatted with Prettier before it can be merged into main. In the meantime, every file that was updated from every other change has been formatted ahead of time.

One of the biggest pain points when working with PRs is figuring out what exactly changed. Having extra lines change formatting, even if they haven't changed behavior, just adds extra noise and makes it harder to focus on what really matters. As Swell continues to grow, it is absolutely vital that all contributors follow strict formatting guidelines, just to keep things manageable. For better or worse, Swell has reached a size where personal style preference cannot have a place in discussions anymore.

The first custom hook has been added

As several of the components have been using the same patterns for managing dropdown state, that functionality has been extracted out into the useDropdownState hook. If any more dropdowns need to be implemented in the future, please use this hook.

What's next

I'm hoping to have two more PRs sent out soon. One will be in charge of enforcing style guidelines in the project, while the other will focus on developer documentation. This PR was just enough of a beast that I wanted to get this one done first, though.

Parkreiner and others added 17 commits August 31, 2023 06:03
This update adds a series of custom alias values
to the Webpack config, to match the import paths
used in the tsconfig file. Together, they finally
enable full support for import paths throughout
the application. This update also adds some basic
type-safety to the Webpack files, too, while keeping
them in the .js format.

Co-authored-by: Travis Lovis <[email protected]>
@Parkreiner Parkreiner changed the title State management finalization State management updates Sep 1, 2023
@Parkreiner Parkreiner changed the title State management updates [WIP] State management updates Sep 1, 2023
@Parkreiner
Copy link
Contributor Author

Closing pull request until discrepancies between local tests and GH Actions tests can be figured out.

@Parkreiner Parkreiner closed this Sep 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant